-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement subscription conflict handling #79
Conversation
Glenn-1990
commented
Jul 26, 2015
m_subscription.state = SUBSCRIPTION_SCRAMBLED; | ||
else if (!strcmp("userLimit", error)) | ||
m_subscription.state = SUBSCRIPTION_USERLIMIT; | ||
else if (!strcmp("noFreeAdapter", error)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we have some time ago a discussion regarding parsing error strings? My mind has not changed since then. Is it guaranteed that these strings are part of tvheadend's API and that nobody will at any time change these strings or that at one day localized strings will appear here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that was the 'status' message, which was a regular string.
Now we use 'subscriptionerror' which represents SM_CODE from tvh in string format, they are not supposed to change at some time in the future, at least that's not why I implemented that.
https://github.com/tvheadend/tvheadend/blob/master/src/htsp_server.c#L3630
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Another piece of code, same story, same design flaw, if you ask me.
Why do you not just return SM_CODE in the tvh API? This would be a much better approach, IMO. What you do here is convert the stuff you translated in tvh from numerical values into strings back into numerical values (maybe even the same numerically as used in tvh - not checked).
Anyhow, it is like it is now as the actual flaw is in tvh. Maybe you take into account what I said (if you can follow my arguments at all) when you add stuff to the tvh API next time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, my first intention was to just sent SM_CODE as integer, but then it came to my mind that the SM_CODE values are just simple defines in tvheadend.h, so they might actually change somewhere in the future. That why I actually added those strings, these strings are not meant to be readable, but only parseble (It even are no sentences).
Only conversion is from int to string on the tvh side.
Seems that I still need this to the hstp specs wiki
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have write access to the tvheadend htsp spec in the wiki. Send me the text and I will update the spec. Then (and only then), the strings are really part of the API and I'm fine with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, my first intention was to just sent SM_CODE as integer, but then it came to my mind that the SM_CODE values are just simple defines in tvheadend.h, so they might actually change somewhere in the future
This is not a matter of data type, it's a matter of putting stuff into the htsp spec. If it's in the spec, it's bug to change it. If it is not in the spec, clients cannot rely on anything, strictly speaking. Again, no matter of data type. Using a string instead of a numeric value does not make things better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ksooo github seems to delete my tabs :s
subscriptionError str optional Subscription error code (Added in version 20).
Note: valid values for subscriptionError
noFreeAdapter No free adapter for this service
scrambled Service is scrambled
badSignal Bad signal status
tuningFailed Tuning of this service failed
subscriptionOverridden Subscription overridden by another one
muxNotEnabled No mux enabled for this service
invalidTarget Invalid target
userAccess User does not have acces rights for this service
userLimit User limit reached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does"Invalid target" mean?
What does "User limit reached" mean?
Could you please extend the descriptions for these two a little to make things more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User limit = each user has a number of maximum streaming connections set in it's user profile, defaulting to unlimited
invalid target = if tvh can not write the actual recording/livestream to the filesystem or when the recording or streaming config is incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Thanks. Added to the spec now. https://tvheadend.org/projects/tvheadend/wiki/Htsp
8b2cea2
to
f55c1f9
Compare
@ksooo reworked, might still need some work I also think that my solution to the no response is not optimal? |
@ksooo I already have a better idea for the sendweight issue, we can do that in the dialog thread, so we won't block anything. @Jalle19 Currently running out of time, so go ahead with re factoring (including SSubscription), I'll rebase later. Actually, if SSubscription has it's own class, things will get easier (i.e. better looking) :p |
void DialogLivestreamAborted( void ) | ||
{ | ||
bDialogForceStart = GUI->Dialog_YesNo_ShowAndGetInput( | ||
XBMC->GetLocalizedString(30461),XBMC->GetLocalizedString(30451), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spaces between parameter xx, xx
@Glenn-1990 only looked at formatting. |
@ksooo I already converted SSubsription from struct to a class, but it's still in htspTypes.cpp, |
Leave it in HTSPTypes.cpp for now. |
f55c1f9
to
8b7e7b6
Compare
m_state = state; | ||
m_mutex.Unlock(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ksooo Need some advise here.
I want to make m_state thread safe, but I do not know much about multi threading, the only thing I know is that I need to protect variables accessed within multiple threads with mutex locks..
m_state is also accessed in GetState(), but that is an const function and const functions are thread safe?
Does this all make sense? :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could take a look at https://github.com/kodi-pvr/pvr.hts/blob/master/src/AsyncState.cpp and https://github.com/kodi-pvr/pvr.vbox/blob/master/src/vbox/StartupStateHandler.h. Your basic idea is correct, you need to use a mutex lock whenever you access the m_state
variable. const
functions are not thread-safe by definition since another thread could come in and overwrite the value of the variable while you're reading it. Since locking a mutex is a non-const operation it is common to mark the mutex variable as mutable so it can be used from const
methods (see e.g. https://github.com/kodi-pvr/pvr.vbox/blob/master/src/vbox/StartupStateHandler.h#L123).
8b7e7b6
to
81e1456
Compare
{ | ||
/* used in multiple threads */ | ||
CLockObject lock(m_mutex); | ||
return m_weight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jalle19 thanks for your help on this.
m_weght, m_state and m_subscriptionId are made threadsafe in the right way I guess? :p
773f880
to
86deb7e
Compare
msgstr "" | ||
|
||
msgctxt "#30451" | ||
msgid "Livestream aborted, adapter stolen by other subscription" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change "Livestream" to "Streaming" or "Live stream"?
@Glenn-1990 any chance you could split the Subscription class refactoring to a separate PR? |
@Jalle19 where should it be placed? Root directory or entity? |
I'd place it either in tvheadend/, it's not really an entity since it doesn't require SetDirty(). |
I'll rebase this when #119 is in |
86deb7e
to
41e4a1b
Compare
00a6451
to
3382f47
Compare
Maybe we should have this as a default disabled setting? |
3382f47
to
f0e18f4
Compare
{ | ||
/* If streaming conflict management enabled */ | ||
if (Settings::GetInstance().GetStreamingConflict()) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would factor out this block to a separate method, e.g. HandleSubscriptionConflict
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, just added HandleConflict() as the class name makes it clear that it's about subscriptions
f0e18f4
to
6c4aa6a
Compare
@Jalle19 What do you think about the English strings in the dialog? |
If you ask me, this stuff is all far too tekky for a normal end user. As I said several times, why don't you just buy a server that fits your needs instead of adding loads of code just dealing with the situation that your server does not have the balls to serve your needs? Not 100% serious about this, but cannot understand your priorities... ;-) I really do not like that you add some real GUI (dialog) to pvr.hts, because I thinks this is no good design. But again, just my 2 Ct, I will not block this PR. |
Well, I'm not facing such a streaming conflict many times as I've a quad tuner, but when I face it, it seems unclear to the user what is happening... I'm not adding a GUI dialog, I just call the kodi one, so no messing with xml files. Maybe we can try to make it simpler? This is a very good example, page 2: http://www.connectctc.com/files/tv-support/Prime-User-Guide/Conflicts/How-to-Resolve-Conflicts.pdf |
Yeah. Simplify it. Especially untekkify the wording. Maybe like in the document you posted. Everything about"stolen adapters" and "priorities" and "hijack" has to go imo, because people will not understand, I'm afraid. Regarding using a dialog. I still have the feeling that conflict handling belongs into Kodi core even though this might be much more work than just hacking this into one add-on. |
I agree about the techiness - the feature is nice though. I don't think it can be inplemented in core Kodi either since very few addons provide the necessary API to even determine if a conflict has taken place. |
I tend to disagree regarding implementing this not in Kodi core. Conflict management is an essential PVR feature and therefore should be handled by Kodi core in a consistent way for all PVR add-ons. The fact that not many servers (today!) offer a suitable API for this is imo unrelated. But again, I'm not going to block this PR if the UI will be changed to be far less techy. |
83042fc
to
021e77e
Compare
@ksooo What do you think of the naming now? |
Is there any chance to present to the user what other conflicting subscriptions are active and let him choose from a list which one to interrupt in order to solve the conflict? This would be cool (and users will understand what's going on). BTW, if I remember correctly, since Isengard we call it just "TV" everywhere in Kodi, not "Live TV", even in English. |
@ksooo Currently that's not possible, or we have to parse the active recordings and check the start time, but that's not a clean solution... Keep in mind that conflict management will be disabled by default. |
021e77e
to
e7eda9b
Compare
@ksooo what if we just show a dialog like this?: And nothing else, so not changing weight etc, just make the user clear what just happened to him. |